fix: use monotonic clock for file processing timeout - #3748
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
CAOShurong
left a comment
CAOShurong
left a comment
There was a problem hiding this comment.
I independently checked the current head 23371b431778b92742b32fc2fec957f7aabee65. On Windows with CPython 3.10, the locked focused file-processing suite passed all 15 tests; Ruff check and format checks passed for both changed files. The sync and async paths both use time.monotonic() consistently, and the regression covers a wall-clock rollback without changing the public timeout API. I found no blocking issue in this focused scope. The full repository CI and matrix remain the appropriate merge gate.
|
Correction: my preceding review contained a typo in the displayed head SHA. The exact head is 23371b4; the review and local checks were run against that commit. |
CAOShurong
left a comment
CAOShurong
left a comment
There was a problem hiding this comment.
Reviewed exact head 23371b4317. The switch from the wall clock to time.monotonic() is the right timeout primitive here, and the parameterized regression exercises both the synchronous and asynchronous helpers under a simulated wall-clock rollback.
Local focused validation on Windows CPython 3.10:
tests/lib/test_file_processing.py: 15 passed
ruff check: passed
ruff format --check: passed
I also verified that both changed file blobs match this exact commit. GitHub currently reports no public check runs for the PR, so this approval is based on the focused local validation above; the repository's required CODEOWNER review remains a separate gate.
Disclosure: I used AI assistance for diff inspection and local test execution; the exact-head checks and results above were independently reviewed.
2 participants
Summary
time.monotonic()for elapsed-time deadlines in both synchronous and asynchronous file-processing polling.Bug
Files.wait_for_processing()and its asynchronous counterpart usedtime.time()to measure elapsed time. If the system wall clock moves backwards while a file is still processing, the calculated elapsed time also moves backwards and the timeout can be postponed indefinitely.Reproduction
With
max_wait_seconds=1,poll_interval=5, and a wall clock that changes from100to90after the first poll:Root cause
Wall-clock time is adjustable and is not suitable for measuring elapsed durations.
Fix
Use Python's monotonic clock for both the initial timestamp and every deadline check. This does not change the public API, poll interval, terminal states, or error message.
Validation
tests/lib/test_file_processing.py: 15 passedCommands used: